Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor: Replace sessions, kernels's status_history's type map with list #1662

Closed
wants to merge 39 commits into from
Closed

refactor: Replace sessions, kernels's status_history's type map with list #1662

wants to merge 39 commits into from

Conversation

jopemachine
Copy link
Member

@jopemachine jopemachine commented Oct 27, 2023 •
edited by github-actions bot
Loading

Refs #412, follow-up to #480.
Related issue: #1116.

The current implementation saves only the most recent timestamp whenever status information in status_history is updated, and all previous information is deleted.

This PR prevents the loss of timestamp information by changing the data structure of sessions, kernels's status_history to List.

Previous format:

{
  "PENDING": "2024-01-01T00:00:05Z",
  "SCHEDULED": "2024-01-01T00:00:10Z",
  "PREPARING": "2024-01-01T00:00:12Z",
  "RESTARTING": "2024-01-01T00:30:00Z",
  "RUNNING": "2024-01-01T00:30:08Z",  // overwritten if restarted
}

New format:

[
  {"status": "PENDING", "timestamp": "2024-01-01T00:00:05Z"},
  {"status": "SCHEDULED", "timestamp": "2024-01-01T00:00:05Z"},
  {"status": "PREPARING", "timestamp": "2024-01-01T00:00:05Z"},
  {"status": "RUNNING", "timestamp": "2024-01-01T00:00:15Z"},  // preserved
  {"status": "RESTARTING", "timestamp": "2024-01-01T00:30:00Z"},
  {"status": "RUNNING", "timestamp": "2024-01-01T00:30:08Z"},
]

Checklist: (if applicable)

  • Milestone metadata specifying the target backport version
  • API server-client counterparts (e.g., manager API -> client SDK)

📚 Documentation preview 📚: https://sorna--1662.org.readthedocs.build/en/1662/

📚 Documentation preview 📚: https://sorna-ko--1662.org.readthedocs.build/ko/1662/

@github-actions github-actions bot added comp:client Related to Client component comp:manager Related to Manager component size:L 100~500 LoC labels Oct 27, 2023
@jopemachine jopemachine changed the title fix: Change status_history from map to list fix: Change status_history from map to list Oct 27, 2023
@jopemachine jopemachine changed the title fix: Change status_history from map to list fix: Replace status_history's type map with list Oct 27, 2023
@jopemachine jopemachine added the require:db-migration Automatically set when alembic migrations are added or updated label Oct 27, 2023
@github-actions github-actions bot added size:XL 500~ LoC size:L 100~500 LoC and removed size:L 100~500 LoC size:XL 500~ LoC labels Oct 30, 2023
@achimnol achimnol added this to the 24.03 milestone Oct 31, 2023
@jopemachine
Copy link
Member Author

jopemachine commented Nov 3, 2023 •
edited
Loading

Tested manually by writing some temporary client code.

    @api_function
    @pass_ctx_obj
    async def get_status_history(ctx: CLIContext, self):
        """
        Retrieves the status transition history of the compute session.
        """

        try:
            with Session() as session:
                fetch_func = lambda pg_offset, pg_size: session.ComputeSession.paginated_list(fields=(
                    session_fields["status_history"],
                    session_fields["status_history_log"],
                ))
                ctx.output.print_paginated_list(
                    fetch_func,
                    initial_page_offset=0,
                    page_size=20,
                )
        except Exception as e:
            ctx.output.print_error(e)
            sys.exit(ExitCode.FAILURE)

The result is as follows.

You can see status_history via graphene still use the same format, and the status_history_log uses the migrationed format.

스크린샷 2023-11-03 오후 12 10 39

@jopemachine jopemachine marked this pull request as ready for review November 6, 2023 01:59
@jopemachine jopemachine force-pushed the fix/status-history branch 2 times, most recently from f73d02d to a94fc0f Compare January 26, 2024 11:22
@achimnol achimnol mentioned this pull request Feb 6, 2024
Open
@jopemachine jopemachine changed the title fix: Replace status_history's type map with list fix: Replace session, kernel's status_history's type map with list Feb 28, 2024
@jopemachine jopemachine changed the title fix: Replace session, kernel's status_history's type map with list fix: Replace sessions, kernels's status_history's type map with list Feb 28, 2024
@achimnol
Copy link
Member

achimnol commented May 3, 2024

We also need to update clients using the session resource usage API handler's status_history field at:

backend.ai/src/ai/backend/manager/api/resource.py

Line 466 in e77c4a3

"status_history": row["status_history"] or {},

as this does not go through the GraphQL's backward-compatible field generation.

@achimnol achimnol modified the milestones: 24.03, 24.09 May 3, 2024
@github-actions github-actions bot added area:docs Documentations comp:common Related to Common component comp:cli Related to CLI component labels May 3, 2024
achimnol
achimnol requested changes May 3, 2024
View reviewed changes
tests/manager/models/test_utils.py Outdated Show resolved Hide resolved
src/ai/backend/manager/models/session.py Outdated Show resolved Hide resolved
changes/1662.fix.md Outdated Show resolved Hide resolved
@jopemachine jopemachine mentioned this pull request May 7, 2024
Closed
2 tasks
@jopemachine jopemachine changed the title fix: Replace sessions, kernels's status_history's type map with list refactor: Replace sessions, kernels's status_history's type map with list May 7, 2024
@jopemachine jopemachine mentioned this pull request May 7, 2024
Open
2 tasks
@jopemachine
Copy link
Member Author

Switching to another PR as we introduce Graphite.
See #2116.

@jopemachine jopemachine closed this May 7, 2024
@Yaminyam Yaminyam deleted the fix/status-history branch July 17, 2024 07:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@achimnol achimnol achimnol requested changes

Assignees

@jopemachine jopemachine

Labels
area:docs Documentations comp:cli Related to CLI component comp:client Related to Client component comp:common Related to Common component comp:manager Related to Manager component require:db-migration Automatically set when alembic migrations are added or updated size:XL 500~ LoC
Projects
None yet
Milestone
24.09
Development

Successfully merging this pull request may close these issues.
2 participants
@jopemachine @achimnol